Introduce Synchronous Currency Conversion Support in Offers #3833
Conversation
|
👋 I see @joostjager was un-assigned. |
|
cc @jkczyz |
|
🔔 1st Reminder Hey @joostjager! This PR has been waiting for your review. |
|
Is this proposed change a response to a request from a specific user/users? |
|
Hi @joostjager! This PR is actually a continuation of the original thread that led to the The motivation behind it was to provide users with the ability to handle So, as a first step, we worked on refactoring most of the Offers-related code out of Hope that gives a clear picture of the intent behind this! Let me know if you have any thoughts or suggestions—would love to hear them. Thanks a lot! |
|
Another use case is Fedimint, where they'll want to include their own payment hash in the |
Does Fedimint plan to use the |
I believe with one. |
|
🔔 2nd Reminder Hey @joostjager! This PR has been waiting for your review. |
|
🔔 3rd Reminder Hey @joostjager! This PR has been waiting for your review. |
|
🔔 4th Reminder Hey @joostjager! This PR has been waiting for your review. |
|
🔔 5th Reminder Hey @joostjager! This PR has been waiting for your review. |
|
🔔 6th Reminder Hey @joostjager! This PR has been waiting for your review. |
|
🔔 7th Reminder Hey @joostjager! This PR has been waiting for your review. |
|
🔔 8th Reminder Hey @joostjager! This PR has been waiting for your review. |
|
🔔 9th Reminder Hey @joostjager! This PR has been waiting for your review. |
|
🔔 10th Reminder Hey @joostjager! This PR has been waiting for your review. |
|
🔔 11th Reminder Hey @joostjager! This PR has been waiting for your review. |
|
Removing @joostjager for now to stop bot spam. @shaavan and I have been working through some variations of this approach. |
lightning/src/offers/flow.rs
Outdated
| amount_source, | ||
| }; | ||
|
|
||
| self.enqueue_offers_event(event)?; |
There was a problem hiding this comment.
Just one comment - is native async in the picture too for this? We've converted other parts of LDK to be native async.
There was a problem hiding this comment.
Hi Joost! In pr3833.02, I’ve scoped this PR down to focus solely on introducing synchronous currency conversion support.
We’re still working through some open questions around the asynchronous path, including whether we can or want to go fully native async, so I’ve left that out for now to keep the scope clean. I’ll keep you posted as the design direction evolves.
Appreciate the nudge, thanks again!
lightning/src/offers/invoice.rs
Outdated
| /// A variant of [`InvoiceBuilder`] that indicates how the signing public key was set. | ||
| /// | ||
| /// This is not exported to bindings users as builder patterns don't map outside of move semantics. | ||
| pub enum InvoiceBuilderVariant<'a> { |
There was a problem hiding this comment.
We probably want to explain also in the docs what explicit and derived mean from an LDK point of view?
Adds the `CurrencyConversion` trait to allow users to define custom logic for converting fiat amounts into millisatoshis (msat). This abstraction lays the groundwork for supporting Offers denominated in fiat currencies, where conversion is inherently context-dependent.
This commit updates the Bolt12Invoice amount creation logic to utilize the `CurrencyConversion` trait, enabling more flexible and customizable handling of fiat-to-msat conversions. Reasoning The `CurrencyConversion` trait is passed upstream into the invoice's amount creation flow, where it is used to interpret the Offer’s currency amount (if present) into millisatoshis. This change establishes a unified mechanism for amount handling—regardless of whether the Offer’s amount is denominated in Bitcoin or fiat, or whether the InvoiceRequest specifies an amount or not.
shaavan
changed the title
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3833 +/- ##
==========================================
- Coverage 88.94% 88.92% -0.03%
==========================================
Files 174 174
Lines 124201 124409 +208
Branches 124201 124409 +208
==========================================
+ Hits 110472 110626 +154
- Misses 11251 11300 +49
- Partials 2478 2483 +5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Following the introduction of the `CurrencyConversion` trait, this commit integrates it into the `Flow` and `ChannelManager` as a required parameter. This change enables users to supply their own fiat-to-msat conversion logic when constructing the Flow or ChannelManager, making synchronised currency handling fully customizable at the application level.
|
Updated from pr3833.01 to pr3833.02 (diff): Changes:
|
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
| Some(Amount::Currency { iso4217_code, amount }) => { | ||
| let unit_msats = currency_conversion.fiat_to_msats(iso4217_code)? * amount; | ||
| unit_msats.checked_mul(quantity).ok_or(Bolt12SemanticError::InvalidAmount) |
There was a problem hiding this comment.
The multiplication of currency_conversion.fiat_to_msats(iso4217_code) * amount could overflow before the subsequent checked_mul(quantity) catches it. Consider using checked_mul for both operations to ensure safe arithmetic:
let unit_msats = currency_conversion.fiat_to_msats(iso4217_code)
.checked_mul(amount)
.ok_or(Bolt12SemanticError::InvalidAmount)?;
unit_msats.checked_mul(quantity).ok_or(Bolt12SemanticError::InvalidAmount)This guards against overflow in both multiplication steps.
| Some(Amount::Currency { iso4217_code, amount }) => { | |
| let unit_msats = currency_conversion.fiat_to_msats(iso4217_code)? * amount; | |
| unit_msats.checked_mul(quantity).ok_or(Bolt12SemanticError::InvalidAmount) | |
| Some(Amount::Currency { iso4217_code, amount }) => { | |
| let unit_msats = currency_conversion.fiat_to_msats(iso4217_code)? | |
| .checked_mul(amount).ok_or(Bolt12SemanticError::InvalidAmount)?; | |
| unit_msats.checked_mul(quantity).ok_or(Bolt12SemanticError::InvalidAmount) |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
There was a problem hiding this comment.
Let's also chain all calls using and_then where needed.
| @@ -2502,6 +2518,7 @@ pub struct ChannelManager< | |||
| F::Target: FeeEstimator, | |||
| R::Target: Router, | |||
| MR::Target: MessageRouter, | |||
| CC::Target: CurrencyConversion, | |||
There was a problem hiding this comment.
I'm not quite remembering our conversations, but why do we need to add a type parameter to ChannelManager? I thought we could have methods on OffersMessageFlow that take a CurrencyConversion and have ChannelManager pass it DefaultCurrencyConversion. Then if a user wants currency conversion, they can request events and pass in their own CurrencyConversion.
| where | ||
| MR::Target: MessageRouter, | ||
| CC::Target: CurrencyConversion, |
There was a problem hiding this comment.
Likewise here. Though I seem to recall chatting about an option where OffersMessageFlow is parameterized by a CurrencyConversion like you have it, but it also would have methods that can be passed a CurrencyConversion for use in the asynchronous event model. In that world, ChannelManager would not be parameterized and would instead hardcode its OffersMessageFlow with DefaultCurrencyConversion.
| created_at: Duration, payment_hash: PaymentHash, signing_pubkey: PublicKey, | ||
| ) -> Result<Self, Bolt12SemanticError> { | ||
| let amount_msats = Self::amount_msats(invoice_request)?; | ||
| pub(super) fn for_offer<CC: core::ops::Deref>( |
| pub fn respond_with( | ||
| &$self, payment_paths: Vec<BlindedPaymentPath>, payment_hash: PaymentHash | ||
| ) -> Result<$builder, Bolt12SemanticError> { | ||
| pub fn respond_with<CC: core::ops::Deref>( |
There was a problem hiding this comment.
Let's import Deref here, too.
| Some(Amount::Currency { iso4217_code, amount }) => { | ||
| let unit_msats = currency_conversion.fiat_to_msats(iso4217_code)? * amount; | ||
| unit_msats.checked_mul(quantity).ok_or(Bolt12SemanticError::InvalidAmount) |
There was a problem hiding this comment.
Let's also chain all calls using and_then where needed.
| /// A trait for converting fiat currencies into millisatoshi values. | ||
| /// | ||
| /// This is used for handling conversions between fiat currencies and Bitcoin denominated in millisatoshis | ||
| /// when working with Bolt12 invoice requests. | ||
| /// | ||
| /// Implementors must provide a method to convert from a specified fiat currency (using ISO 4217 currency codes) | ||
| /// to millisatoshis, handling any potential conversion errors. |
There was a problem hiding this comment.
Please wrap all documentation at 100 characters.
| /// to millisatoshis, handling any potential conversion errors. | ||
| pub trait CurrencyConversion { | ||
| /// Converts a fiat currency specified by its ISO 4217 code to millisatoshis. | ||
| fn fiat_to_msats(&self, iso4217_code: CurrencyCode) -> Result<u64, Bolt12SemanticError>; |
There was a problem hiding this comment.
Let's make sure this is well documented. The conversion needs to the adjust for the ISO 4217 currency exponent. For example, a conversion from USD needs to be in terms of msats per cent (i.e., 1/100 of a dollar) not per dollar.
| @@ -1876,7 +1895,12 @@ mod tests { | |||
| .unwrap() | |||
| .build_and_sign() | |||
| .unwrap() | |||
| .respond_with_no_std(payment_paths.clone(), payment_hash, now) | |||
| .respond_with_no_std( | |||
| &DefaultCurrencyConversion {}, | |||
There was a problem hiding this comment.
Could we define an exchange_rate test utility to avoid (most) line wrappings?
This PR introduces support for Offers that specify amounts in fiat currencies by adding a synchronous currency conversion mechanism to the
Flow.To enable this, it defines a new
CurrencyConversiontrait, allowing users to provide custom fiat-to-msat conversion logic. This trait is threaded upstream into bothFlowandChannelManager, enabling unified handling of amount creation and verification, regardless of whether the Offer amount is denominated in Bitcoin or fiat.The approach is intentionally synchronous, focusing on minimal, clean, and maintainable changes. While an synchronous model (e.g., event-driven just-in-time conversion or injecting custom
payment_hash) is also being explored, it adds considerable complexity. By introducing currency support in a synchronous form first, this PR cleanly isolates the core functionality from broader architectural changes.The trait exposes a single fiat-to-msat conversion function, where the user supplies a unit multiplier (e.g., msats per USD). All other logic, such as scaling by quantity and internal amount construction, is handled internally. This design reduces ambiguity, ensures correctness, and keeps the integration lightweight for the user.
In essence, this PR lays the foundation for robust fiat-denominated Offer support in a way that is simple, extensible, and forward-compatible.